Fix incorrect textPixelRatio argument #6135
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
February 12, 2018 16:30
Closes #6075 `textPixelRatio` is used to scale tile coordinates to viewport coordinates; the value that was being passed here, EXTENT / tileSize, was the reciprocal of the ratio needed -- we were passing 16 instead of 0.0625. As a result, the corners of the viewport-projected collision box that we reconstructed here, https://github.com/mapbox/mapbox-gl-js/blob/f13c86ea356c384fdab31855b9152f5bf5ef97b8/src/symbol/collision_index.js#L322-L325, were 16^2 times farther from the anchor than they should have been. With `icon-offset: [0, 0]`, this didn't matter, because the incorrectly large box would contain the (much smaller) correct box. But when there's an offset, it too gets scaled too much, and so we run out of luck.
ChrisLoer
approved these changes
Feb 12, 2018
There was a problem hiding this comment.
🤦♂️ Good catch @anandthakker. This textPixelRatio has always been incorrect, but it used to be masked by us inverting the tileToViewport parameter. When I fixed that in #5913, I missed this.
|
Although this change is the simplest fix, it's worth noting that on gl-native we skipped this issue entirely by storing viewport coordinates in the |
pathmapper
pushed a commit
to pathmapper/mapbox-gl-js
that referenced
this pull request
Feb 13, 2018
* Add failing test for issue mapbox#6075 * Fix incorrect textPixelRatio argument Closes mapbox#6075 `textPixelRatio` is used to scale tile coordinates to viewport coordinates; the value that was being passed here, EXTENT / tileSize, was the reciprocal of the ratio needed -- we were passing 16 instead of 0.0625. As a result, the corners of the viewport-projected collision box that we reconstructed here, https://github.com/mapbox/mapbox-gl-js/blob/f13c86ea356c384fdab31855b9152f5bf5ef97b8/src/symbol/collision_index.js#L322-L325, were 16^2 times farther from the anchor than they should have been. With `icon-offset: [0, 0]`, this didn't matter, because the incorrectly large box would contain the (much smaller) correct box. But when there's an offset, it too gets scaled too much, and so we run out of luck.
pathmapper
pushed a commit
to pathmapper/mapbox-gl-js
that referenced
this pull request
Feb 13, 2018
* Add failing test for issue mapbox#6075 * Fix incorrect textPixelRatio argument Closes mapbox#6075 `textPixelRatio` is used to scale tile coordinates to viewport coordinates; the value that was being passed here, EXTENT / tileSize, was the reciprocal of the ratio needed -- we were passing 16 instead of 0.0625. As a result, the corners of the viewport-projected collision box that we reconstructed here, https://github.com/mapbox/mapbox-gl-js/blob/f13c86ea356c384fdab31855b9152f5bf5ef97b8/src/symbol/collision_index.js#L322-L325, were 16^2 times farther from the anchor than they should have been. With `icon-offset: [0, 0]`, this didn't matter, because the incorrectly large box would contain the (much smaller) correct box. But when there's an offset, it too gets scaled too much, and so we run out of luck.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #6075
textPixelRatiois used to scale tile coordinates to viewport coordinates; the value that was being passed here, EXTENT / tileSize, was the reciprocal of the ratio needed -- we were passing 16 instead of 0.0625.As a result, the corners of the viewport-projected collision box that we reconstructed here,
mapbox-gl-js/src/symbol/collision_index.js
Lines 322 to 325 in f13c86e
icon-offset: [0, 0], this didn't matter, because the incorrectly large box would contain the (much smaller) correct box. But when there's an offset, it too gets scaled too much, and so we run out of luck.